Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[POC/WIP] Angular component with redux settings #1944

Closed

Conversation

karelhala
Copy link
Contributor

@karelhala karelhala commented Aug 17, 2017

This PR introduces basic settings of redux in MiQ repository. In future redux will be in separate repository which will be rails plugin and also available as NPM package, so other plugins can include such package.

Working together with @vojtechszocs

Work very much in progress, so do not merge this.

EDIT:
Part of this PR is redux related stuff and some of it is using it from angular when creating basic form. This PR will later down the line drop redux stuff, which will be moved to it's own repository. However using redux from angular needs some explanation.

Using redux in angular component written in TS.

  1. We introduced new class which should be base class for every form written in TS and using angular. app/javascript/forms-common/defaultFormController.ts
    This abstract class will bind it's child class to redux store, if you'll pass any reducers hash it will apply them to rootReducer.
  2. We introduced new way of creating reducers, as you can see in file app/javascript/middleware/forms/new-provider-reducer.ts. It is not required to be done this way, but it's somewhat easier to read than using big switch statement.
  3. To react to state changes you should create reducer which listens to UPDATE_FORM action and assign payload to correct place in state tree.
  4. If some fields need default values there is INIT_FORM action, which is fired inside $onInit.
  5. Controller which is taking care of form component HAS to implement updateFormObject method, which is fired every time state changes.
  6. To connect readux state form changes in template assign some part of state object to formObject as seen in updateFormObject method.
  7. To update state, you should fire onChangeForm function using ng-change

Default actions

Since there can be only one form per page and since when registering to rootReducer we provide callback to delete this reducer from rootReducer we can have couple of default actions which can forms use.

  • INIT_FORM - for setting default value to form object. Also create form object in state tree.
  • UPDATE_FORM - fired when field in form is updated. Can be fired using method onChangeForm which will pass formObject as payload.

Reducers hash

To ease creation of reducers we introduced somewhat different way of creating reducers, you don't have to write complex switch statement, which can be hard to read and big and complex reducers can lead to slower state propagation. You will specify object with actions as keys and values will be function or function callbacks. For example take a look at simple reducer with one function callback and one inline function.

function functionCallback(state, action) {
  //Do something with state
  return {...state};
}

{
  'ACTION_TYPE': functionCallback,
  'ACTION_TYPE_2': function(state, action) {
    //do something with state;
    return {...state};
  }
}

Extensible components

If some component decides that it would like to be extended by some plugin, it can use new feature called extensible component. It sits under ManageIQ.extensionComponents where you have 2 things:

  • items - set of items registered for extensions
  • source - observable to subscribe to and to send extensible components trough

So to register some component to be extensible call

ManageIQ.extensionComponents.next(
  {
    action: 'add',
    payload: {
      name: 'name-of-component',
      api: {some: callbacks()},
      render: {some: renderCallbacks()}}
  }
)

And if some plugin wants to extend such component, just call (inside plugin) ManageIQ.extensionComponents.source.subscribe() with some inner logic.

@@ -0,0 +1,14 @@
import {createStore} from 'redux';
import {addReducer, applyReducerHash} from './reducer';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: inconsistent formatting/spacing, maybe consider using something like prettier or some other editor configuration :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or tslint.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fon't know why, but for some reason my VS code did not picked up .editorconfig and went on it's own with formatting. But I will definetely use tslint to be mean to everyone if they'll make any mistake.

import { AppState } from '../packs/miq-redux';

const asdf = 'INIT_NEW_PROVIDER_HAWKULAR'
function initNewProvider(state, action): AppState {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick - can be written as

const initNewProvider = (state, action): AppState => (
  {...state, data: action.payload.data}
);

see https://egghead.io/lessons/javascript-redux-simplifying-the-arrow-functions

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or tassign which is the typesafe version of Object.assign so it will only allow valid properties to be used.
https://www.npmjs.com/package/tassign

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally like a lot the spread operator, but in this case I was mostly referring to arrow functions vs functions (that require return etc)

return newState;
};

export function addReducer(reducer: AppReducer) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: inconsistent, once using a function another time es6 :)

let newState = state;

reducers.forEach((reducer) => {
newState = reducer(newState, action)
Copy link
Member

@ohadlevy ohadlevy Aug 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand this, why not use combineReducers which just accepts all reducers?

I don't think you are required to loop over all reducers yourself?
Also, I can see nested reducers (e.g. many levels deep).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We basically try to aim for simplicity and flexibility.

The addReducer (miq-redux) function is to be used by both core (manageiq-ui-classic) and plugins. You start with no reducers (empty Set) and allow client code to add new ones.

In a component-oriented architecture with Redux, you have specific components bound to (or data-driven by) specific parts of the state tree. Having one reducer per one logical component seems like a natural fit for such architecture.

As you wrote, one would normally use Redux combineReducers helper to combine smaller reducers into bigger ones. The difference here is that we don't (and shouldn't) know in advance what the specific reducers will be. We simply provide a way to add your reducer via addReducer. This means the MiQ Redux infra is opt-in by design, allowing gradual adoption of Redux architecture, typically on per-component basis.

Calling all reducers in a forEach loop reflects the design that "all reducers are on the same level" - they all get the whole state, the action that was dispatched, and impose changes on the relevant part of the state tree (while following the general principle of state immutability). This design follows the goals mentioned above - simplicity and flexibility.

We could, by design, constrain each reducer to a specific state subtree. It would probably result in API like addReducer(myReducer, 'foo.bar') where myReducer would be constrained to operate only on a subtree represented by the expression state.foo.bar. But do we really need that?

All reducers, both core and plugin contributed, are to be treated in the same way from MiQ Redux infra perspective. If a reducer does something harmful, like overwriting a specific state subtree in an inconsistent way, it's a problem with that particular reducer (or generally with the code that added such reducer).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the detailed answer. I don't see the two conflict. E.g. we should have an API to add redcuers (or a whole bunch of reducers) but then pass it (array of reducer functions) as argument to combine reducers which will do the actual looping and basic tests (return values etc).

This will easily allow you to create sub reducers naturally (which I can't are us going without)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ohadlevy thanks for the link. I'd prefer the API to be as simple as possible 😃 I guess we should give it some more thought.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vojtechszocs I don't think that changes the API rather how its implemented?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it depends on how the API would look like. Right now, there's a simple addReducer function. Can you share some code snippet of how you'd imagine the reducer composition in MiQ Redux infra?

@mzazrivec mzazrivec added the wip label Aug 18, 2017

import {AppState} from '../packs/miq-redux';

const composeEnhancers = window['.__REDUX_DEVTOOLS_EXTENSION_COMPOSE__'] || compose;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra dot before __REDUX ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good one. Did not used the browser's febugger, yet. But I'd definetely wonder why it's not working.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should be

window.__REDUX_DEVTOOLS_EXTENSION__ && window.__REDUX_DEVTOOLS_EXTENSION__(),

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We were planning ahead and added support for middlewares as well, and based on their docs we should use advanced-store-setup with middlewares.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are correct, I didnt see that example, in general its not required if you don't provide an initialState but that make sense long term too (caching in session storage etc).

just a question, why do you access it via [] vs

const composeEnhancers = window.__REDUX_DEVTOOLS_EXTENSION_COMPOSE__ || compose;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because typescript is sometimes too aggressive and was complaining that __REDUX_DEVTOOLS_EXTENSION_COMPOSE__ does not exists on window.

But since you brought it up, which one would you prefer? Because I am Indecisive between accessing it via [] or re-typing window object to any.

const composeEnhancers = (<any>window).__REDUX_DEVTOOLS_EXTENSION_COMPOSE__ || compose;

Copy link
Contributor

@vojtechszocs vojtechszocs Aug 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the __REDUX_DEVTOOLS_EXTENSION_COMPOSE__ global is well-known in advance, I'd prefer the following form:

window.__REDUX_DEVTOOLS_EXTENSION_COMPOSE__

and tell TypeScript to accept (recognize) that global. I'd use the [] access mainly for dynamic properties.

Since Redux compose returns a function, the TypeScript-annotated property access should be (<function>window).

Anyway, I think the proper solution should be to declare this global using namespace so that one doesn't have to always use (<function>window) access prefix.

@himdel
Copy link
Contributor

himdel commented Aug 18, 2017

Before removing wip, please make sure to use 2 spaces indent consistently, not 4, not 8 :).
Also, newlines at the end of files.. looks like you may need to enable .editorconfig support in your editors ;).

@ohadlevy
Copy link
Member

@himdel how would you feel we add hound support for js and css?

@karelhala
Copy link
Contributor Author

@himdel this PR will proably be not part of ui-classic, but those 4 spaces bugs me as well. Btw what do you and @martinpovolny think about moving this to a separate repo which will be rails plugin and also npm package?

@himdel
Copy link
Contributor

himdel commented Aug 21, 2017

@ohadlevy Hound probably doesn't bring us anything (or does it?).. we're already using codeclimate (and a bunch of others)..

I think the problem is that the existing eslint rules apply only to app/assets/javascripts, what's missing now is a proper ES6 aware eslint config for app/javascript.. and I'm really hoping eslint does/will have support for typescript.. it feels a bit silly to try to muck about with tslint rules when the whole of JS world has pretty much settled on eslint already (and their parser does understand typescript now (babel/babylon#523)). Also, tslint doesn't quite support all the rules we're checking (see the list in https://github.com/buzinas/tslint-eslint-rules) ..

EDIT: I guess what I'm saying is that CI support is secondary to figuring out the right way to lint TS first :)

@karelhala
Copy link
Contributor Author

@himdel that's quite easy to do, we should use eslint-loader, plus adding .eslintrc.json to app/javascript.

@himdel
Copy link
Contributor

himdel commented Aug 21, 2017

Adding eslintrc.json to app/javascript is clear, yeah, we were using rules based on eslint-config-airbnb5 with a few of our changes, from the discussion in ManageIQ/manageiq#8781.

So the es6 version would be probably based off eslint-config-airbnb ,and the rest of the rules can probably stay the same.


As for eslint-loader... Please don't :). When you're developing, it can get pretty annoying that you can't test your code just because webpack failed on a missing semicolon. Let's leave this to editors and codeclimate.

@himdel
Copy link
Contributor

himdel commented Aug 21, 2017

.. But tested that current eslint can actually handle .ts code just fine :)

So.. doing a PR to add that eslintrc.json and we'll see what happens :)

@vojtechszocs
Copy link
Contributor

Hi everyone, here are some thoughts after reading the recent comments.


The initial addReducer API is very simple and reflects the design that all reducers operate on the whole state. @ohadlevy raised a comment regarding reducer composition, which generally means that reducers should be scoped to a particular state subtree.

I'm all for reducer composition given that its implementation for our Redux infra is simple enough. If you look at Redux combineReducers, you'll notice that it only supports a single composition level, plus it makes following assumptions:

  • all reducers are known in advance, passed via reducers argument
  • because of that, change detection (hasChanged) is easy to implement
  • if there was a change, a new object (nextState = {}) is always returned (which generally means a change in subtree implies change in all parents of that subtree)

Proposed MiQ Redux infra allows dynamic addition of reducers, therefore the "reducers are known in advance" doesn't hold, which in turn complicates change detection and reducer composition in general. In other words, MiQ Redux infra is not a typical Redux application (not SPA, not fully componentized and therefore not fully Redux-aware).


I agree with @ohadlevy about using mapToProps-like approach for the given component's state change listener:

  • take the whole state as input, select bits of data (important for that component) using e.g. reselect
  • map the important bits of data into "props" (bindings) to be passed into the actual component

If you plan to test your Angular components, it's a good idea to separate the "connect to Redux" concern by writing a higher-order (also known as "connected") component.

@ohadlevy
Copy link
Member

ohadlevy commented Aug 22, 2017

I'm all for reducer composition given that its implementation for our Redux infra is simple enough. If you look at Redux combineReducers, you'll notice that it only supports a single composition level, plus it makes following assumptions:

all reducers are known in advance, passed via reducers argument
because of that, change detection (hasChanged) is easy to implement
if there was a change, a new object (nextState = {}) is always returned (which generally means a change in subtree implies change in all parents of that subtree)
Proposed MiQ Redux infra allows dynamic addition of reducers, therefore the "reducers are known in advance" doesn't hold, which in turn complicates change detection and reducer composition in general. In other words, MiQ Redux infra is not a typical Redux application (not SPA, not fully componentized and therefore not fully Redux-aware).

Why can't we assume the reducers are registered at some "app startup time" e.g. I can't see a reducer being added dynamically (just like a plugin won't be added on the fly) ?

In that case, IMHO we should be able to know the reducers in advance (and that's why I suggested the registration / factory pattern).

I agree with @ohadlevy about using mapToProps-like approach for the given component's state change listener:

take the whole state as input, select bits of data (important for that component) using e.g. reselect
map the important bits of data into "props" (bindings) to be passed into the actual component
If you plan to test your Angular components, it's a good idea to separate the "connect to Redux" concern by writing a higher-order (also known as "connected") component.

👍 this will make our angular / react implementations to be fully disconnected from the store structure, allowing us to refactor in the future, or in other words, angular code should never talk to the store regardless.

@vojtechszocs
Copy link
Contributor

As for reducer composition, I gave it some more thought. Redux createStore function fires all state change listeners regardless of whether the state has changed or not. Simply put, dispatching an action to Redux store always fires all registered listeners.

Therefore, we can come up with some API that allows reducer composition (effectively scoping reducers to specific state subtrees) while not worrying too much about state change detection. All listeners attached to MiQ Redux store should use reselect or similar to avoid recomputations when data doesn't change, anyway.

@vojtechszocs
Copy link
Contributor

Why can't we assume the reducers are registered at some "app startup time" e.g. I can't see a reducer being added dynamically (just like a plugin won't be added on the fly) ?

I'd prefer not to make such assumption. Lazy loading (code splitting) reducers is a valid use case.

All JS packs (core and plugin contributed) execute after core MiQ JS code. By defining "app startup time" we remove some flexibility.

@ohadlevy
Copy link
Member

I'd prefer not to make such assumption. Lazy loading (code splitting) reducers is a valid use case.

Is it? TBH I can't see that ?

All JS packs (core and plugin contributed) execute after core MiQ JS code. By defining "app startup time" we remove some flexibility.

On the other hand we embrace a journey that is different than many other apps using this technology, so before we go on that path, we are doing it because we have the correct requirements?

@vojtechszocs
Copy link
Contributor

@ohadlevy we can't see yet how MiQ Redux infra will be used, we're just designing it now and understanding our requirements, based on components we're developing (new middleware form - in progress). I'd suggest to keep the most simple and flexible design until we have a couple of Angular components connected to Redux store, extended using e.g. React technology and having those extensions also connected to Redux store as well. With that done, we will have a better picture of what we need & how we actually use the Redux infra.

I've also discovered an interesting project called extensible-duck which defines a standard way to bundle reducer, action types & action creators in a single logical package, aka the "duck". We might end up doing something similar, but at this point, I'd keep things as simple as possible. What drives our requirements are (will be) the Angular components and their extensions, I think.

@karelhala karelhala force-pushed the angularNewProviderMiddleware branch 2 times, most recently from d4431c4 to 70c8b1c Compare August 28, 2017 15:01
@karelhala
Copy link
Contributor Author

I take it as that the redux part is solid and can be used like this, am I correct? If so, we should move redux related stuff to it's own repo which will have npm package settings and it will be ruby plugin based on https://github.com/martinpovolny/miq_plugin_example. However this can be done later on.

PR definition has been updated to correspond current state.

@karelhala
Copy link
Contributor Author

@vojtechszocs take a look at app/javascript/extensible-components/index.ts and app/javascript/extensible-components/lib.ts. It's partly the implementation of extensible component which we discussed together.

@vojtechszocs
Copy link
Contributor

@karelhala - I did some code improvements on top of your commits, see here and here. I didn't push them into this PR yet, let me know what you think.

I take it as that the redux part is solid and can be used like this, am I correct?

I think the MiQ Redux part is simple and solid enough for us to proceed further. I see several possible improvements, but these can be done later on. Some ideas for the future:

1, Allow reducers to be scoped to a particular state subtree. For example, all reducers defined in new-provider-reducer.ts would operate on providers.middleware.hawkular.newProvider subtree of the whole state. This could be realized by extending existing addReducer API signature, for example:

addReducer(reducer, 'providers.middleware.hawkular.newProvider')

2, As suggested by @ohadlevy - draw inspiration from React world: whenever Redux state changes, map state object into necessary bits of data (similar to react-redux mapStateToProps function) and re-render the given component.

In this PR, we use Angular v1 component helper with controllerAs option, effectively routing component's changes through a local state (model) object, as defined in HTML template:

"ng-model"    => "newProv.componentState.name",
"ng-value"    => "newProv.componentState.name",

But this is due to using Angular v1. In Angular latest, there should be no local (component-specific) state. Additionally, we should use libraries like reselect to compute any derived data, as well as memoizing any state projections for better performance.

If so, we should move redux related stuff to it's own repo

Yes, that's the future. I'd recommend to keep hacking this PR a bit more to cover component extension & reusability as well.

From MiQ core perspective, it will be a Rails plugin that contributes all the infra & APIs through Webpacker's app/javascript/packs mechanism.

From MiQ (JavaScript) plugin perspective, it will be a monorepo (repo hosting multiple npm packages):

  • manageiq/redux package, accompanied by @types/manageiq-redux to be hosted through DefinitelyTyped repo
  • similar as above, but for component extension & reusability

We can consider using tools like Lerna to manage the monorepo.

PR definition has been updated to correspond current state.

Looks good to me! If I had to generalize the Redux workflow for form-based components, I'd say that:

  • each form may dispatch an INIT action inside its $onInit hook
  • each form should delegate all changes to Redux store: either use a single (general-purpose) UPDATE action, or use multiple fine-grained actions, whatever is more appropriate
  • each form should reflect the Redux state into its view (HTML) whenever the Redux state changes

Since there can be only one form per page

Given that condition, we can have a single pair of INIT and UPDATE actions for form-based components, as you suggested in PR description.

I'm going to review the latest commit that addresses component extension.

PS: sorry for my late response, I was out during the recent days.

@karelhala karelhala force-pushed the angularNewProviderMiddleware branch 2 times, most recently from 89417a1 to 64fa5dd Compare September 6, 2017 15:43
@karelhala
Copy link
Contributor Author

Don't get scared that there are extensible component commits as well. This PR relies on it, so it's rebased on top of it. Once #2060 will be merged these commits will not be here.

@miq-bot
Copy link
Member

miq-bot commented Sep 13, 2017

Checked commits karelhala/manageiq-ui-classic@6d7d3e2~...c62816c with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 2 offenses detected

app/views/ems_middleware/new.html.haml

  • ⚠️ - Line 17 - Prefer to_s over string interpolation.
  • ⚠️ - Line 17 - Prefer to_s over string interpolation.

@vojtechszocs
Copy link
Contributor

Adding link to redux-form which is React-based, but we can still draw inspiration from it. It's basically a library to connect existing form components to a Redux store. See it in action here.

The fundamental difference between redux-form and our own Redux infra is that redux-form includes:

  • (1) UI (React) specific component helpers for both the form itself and its individual fields
  • (2) having a single formReducer to cover all the forms

For Angular 1.x form component impl. like in this PR, we can consider doing (2) above, while also exploring the possibility of having field-specific lifecycle hooks like format, parse, normalize etc.

Therefore, I'd suggest to split this PR into two commits, one for the Redux infra itself, and one for Angular-specific helpers that connect Angular components to a Redux store.

@miq-bot
Copy link
Member

miq-bot commented Oct 5, 2017

This pull request is not mergeable. Please rebase and repush.

@martinpovolny
Copy link
Member

@karelhala, @vojtechszocs : what is the status of this PR?

@miq-bot
Copy link
Member

miq-bot commented Jun 25, 2018

This pull request has been automatically closed because it has not been updated for at least 6 months.

Feel free to reopen this pull request if these changes are still valid.

Thank you for all your contributions!

@miq-bot miq-bot closed this Jun 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants